Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve concurrency and cache for kubelet credential provider #102168

Merged
merged 1 commit into from
Jul 2, 2021
Merged

Improve concurrency and cache for kubelet credential provider #102168

merged 1 commit into from
Jul 2, 2021

Conversation

adisky
Copy link
Contributor

@adisky adisky commented May 20, 2021 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Improve concurrency and cache for credential provider

This is a part of beta requirement, This PR addresses some concurrency concerns that are required for beta.

Removed lock from "Provide" as it can be called in parallel from image puller. To avoid calling exec for the same image concurrently wrapped exec in singleflight.

Purging the cache for expired data with 15mins interval only when a request for credential is made.

Which issue(s) this PR fixes:

part of: kubernetes/enhancements#2133

Special notes for your reviewer:

Addresses part of https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2133-kubelet-credential-providers#alpha---beta-graduation

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]:https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2133-kubelet-credential-providers

/sig node auth cloud-provider

Signed-off-by: Aditi Sharma adi.sky17@gmail.com

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 20, 2021
@k8s-ci-robot k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label May 20, 2021
@adisky
Copy link
Contributor Author

adisky commented May 20, 2021

/cc @andrewsykim @liggitt

@enj enj added this to Needs Triage PRs in SIG Auth Old May 20, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2021
@adisky adisky force-pushed the credential-provider-1 branch 4 times, most recently from 4ad9be3 to 0d11d2e Compare May 21, 2021 16:09
@adisky adisky changed the title Improve concurrency and cache for kubelet credential provider [WIP] Improve concurrency and cache for kubelet credential provider May 21, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2021
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 16, 2021
@JamesLaverack JamesLaverack mentioned this pull request Jun 21, 2021
Closed
4 tasks
enj
enj suggested changes Jun 22, 2021
View reviewed changes
pkg/credentialprovider/plugin/plugin_test.go Outdated Show resolved Hide resolved
pkg/credentialprovider/plugin/plugin.go Show resolved Hide resolved
SIG Auth Old automation moved this from In Review (v1.22) to Changes Requested (v1.22) Jun 22, 2021
@enj
Copy link
Member

enj commented Jun 22, 2021

Posting down here to make sure it does not get lost: #102168 (comment)

@adisky adisky force-pushed the credential-provider-1 branch 2 times, most recently from 647f36e to 2ecc76f Compare June 23, 2021 11:42
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2021
@cheftako
Copy link
Member

/cc @DangerOnTheRanger

@adisky
Copy link
Contributor Author

adisky commented Jun 28, 2021

/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-node-e2e-containerd

@adisky
Copy link
Contributor Author

adisky commented Jun 28, 2021

/cc @liggitt Please take a look again

@k8s-ci-robot
Copy link
Contributor

@adisky: GitHub didn't allow me to request PR reviews from the following users: Please, take, a, look, again.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @liggitt Please take a look again

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@adisky
Kubelet Credential Provider
def9331 
Improve concurrency and cache for credential provider

Removed lock from "Provide" as it can be called in parallel
from image puller. To avoid execing for the same image concurrently
wrapped exec in singleflight.

Purging the cache for expried data with 15mins interval only when
a request for credential is made.

KEP:2133

Signed-off-by: Aditi Sharma <adi.sky17@gmail.com>
andrewsykim
andrewsykim reviewed Jul 1, 2021
View reviewed changes
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2021
@liggitt
Copy link
Member

liggitt commented Jul 2, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adisky, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 2, 2021 •
edited

@adisky: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e-alpha 1d223114d0bfa4fa97506a2c1f931a014fc8aa8e link /test pull-kubernetes-node-e2e-alpha

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@pacoxu
Copy link
Member

pacoxu commented Jul 2, 2021

/retest

@k8s-ci-robot k8s-ci-robot merged commit 3e0432c into kubernetes:master Jul 2, 2021
SIG Auth Old automation moved this from Changes Requested (v1.22) to Closed / Done Jul 2, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jul 2, 2021
@adisky adisky deleted the credential-provider-1 branch April 4, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehashman ehashman ehashman left review comments

@enj enj enj requested changes

@andrewsykim andrewsykim andrewsykim left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@mcrute mcrute Awaiting requested review from mcrute

@DangerOnTheRanger DangerOnTheRanger Awaiting requested review from DangerOnTheRanger

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG Auth Old
Closed / Done
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@adisky @liggitt @ehashman @enj @cheftako @k8s-ci-robot @pacoxu @andrewsykim